Chore: refactor Comparison out of QueryPlanSerde #2028
Conversation
- Create comparisons.scala following the pattern from math/array expressions. - Implements CometGreaterThan as proof of concept for issue apache#2019.
CuteChuanChuan
force-pushed
the
raymond/refactor-expressions-in-QueryPlanSerde
branch
from
56ddd51 to
202556d
Compare
- Add ComparisonBase trait with reusable helper methods - Move GreaterThan, GreaterThanOrEqual, LessThan, LessThanOrEqual to CometComparison* - Move IsNull, IsNotNull, IsNaN to CometIs* classes - Move In expression to CometIn class - Update exprSerdeMap to use new comparison classes - Remove corresponding case statements from exprToProtoInternal
CuteChuanChuan
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2028 +/- ##
============================================
+ Coverage 56.12% 58.77% +2.64%
- Complexity 976 1253 +277
============================================
Files 119 135 +16
Lines 11743 13159 +1416
Branches 2251 2392 +141
============================================
+ Hits 6591 7734 +1143
- Misses 4012 4186 +174
- Partials 1140 1239 +99 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @andygrove , Could you please take a look at this PR and provide some guidance when you have a chance? I've refactored the comparison expressions (GreaterThan, LessThan, IsNull, In, etc.) following the pattern discussed in #2019. In the meantime, I'll continue working on refactoring the remaining expressions from the list. Thanks! |
There was a problem hiding this comment.
Thanks for the first contribution @CuteChuanChuan. First feedback provided below.
| * @return | ||
| * Some(Expr) or None if not supported | ||
| */ | ||
| def createUnaryExpr( |
There was a problem hiding this comment.
Why do we duplicate createUnaryExpr and createBinaryExpr here?
| } | ||
| } | ||
|
|
||
| def in( |
There was a problem hiding this comment.
This is only called in one location. Can we move this logic to the call site in CometIn since it seems specific to that expression?
Which issue does this PR close?
Part of #2019
Rationale for this change
See #2019 for rationale.
What changes are included in this PR?
Moved comparison expressions to separate file (comparison.scala)
How are these changes tested?
There are no functional changes, so it relies on existing tests.
Details: